Conversation
| java.util.List<String> labelValues = getLabelValues(channel); | ||
| try { | ||
| if (channel.getClass().getName().equals(epollSocketChannelClassName)) { | ||
| Class<?> tcpInfoClass = Class.forName(epollTcpInfoClassName); |
There was a problem hiding this comment.
This is quite a lot of reflection in hot exporting path. So, a couple of questions.
- Why do we choose reflectin over typecasting and calling actual methods? Reducing dependency surface?
- Is there a way to optimise this like caching value on per channel level? Something like a creating a runnable per channel. Not sure how it'd be possible to share this data for the final collection on channel inactive.
| Method rttMethod = tcpInfoClass.getMethod("rtt"); | ||
|
|
||
| long totalRetrans = (Long) totalRetransMethod.invoke(info); | ||
| int retransmits = (Integer) retransmitsMethod.invoke(info); |
There was a problem hiding this comment.
Are these cumulative metrics or are these total retransmits since the connection start? If latter, the current logic is incorrect.
There was a problem hiding this comment.
AAh I see, then I think we should not be using a counter for this... Will clarify in the gRFC
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
"otel" is a very surprising prefix to use for the PR/commit, as opentelemetry isn't actually changed at all.
core/src/main/java/io/grpc/internal/ClientTransportFactory.java
Outdated
Show resolved
Hide resolved
…ddress PR comments)
The backoff timer is only used when serializeRetries=true, and that exists to match the old/current pick_first's behavior as closely as possible. InternalSubchannel.updateAddresses() would take no action when in TRANSIENT_FAILURE; it would update the addresses and just wait for the backoff timer to expire. Note that this only impacts serializeRetries=true; in the other cases we do want to start trying to the new addresses immediately, because the backoff timers are in the subchannels. Note that this change was also important because requestConnection() can be directly triggered by the user with channel.getState(true), and that shouldn't defeat the backoff timer.
Since we're only supporting API levels 23+, all the supported Android versions handle multidex natively, and without any bugs to workaround. Also bump some minSdkVersion that didn't get updated in fa7b52b so that multiDex is actually enabled by default. See also b/476359563
… and 'core: Wait for backoff timer on address update in pick_first'
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
The PR description's "Implements A80" doesn't link to the gRFC; it just links to this PR.
netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java
Outdated
Show resolved
Hide resolved
servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
| }); | ||
| this.tcpMetrics = new TcpMetrics(metricRecorder); |
There was a problem hiding this comment.
Move this next to the other assignments. Once we register listeners, we can get callbacks on those listeners. That's not that high of a risk with Netty, but seems needless. Also, this line is not at all obvious after that big Http2ConnectionAdapter implementation, without even an empty line.
| List<? extends ServerStreamTracer.Factory> streamTracerFactories) { | ||
| return buildTransportServers(streamTracerFactories); | ||
| List<? extends ServerStreamTracer.Factory> streamTracerFactories, | ||
| io.grpc.MetricRecorder metricRecorder) { |
|
|
||
| static final class EpollInfo { | ||
| final Class<?> channelClass; | ||
| final Class<?> infoClass; |
|
|
||
| @Before | ||
| public void setUp() throws Exception { | ||
| TcpMetrics.epollInfo = TcpMetrics.loadEpollInfo(); |
There was a problem hiding this comment.
This needs to be in an @After. The last test that runs will leave the test value around.
3 participants
Implements A80